Skip to content

Conversation

@sid597
Copy link
Collaborator

@sid597 sid597 commented Jan 10, 2026

currently findDiscourseNode exists if we don't pass the uid which is wrong because both uid and title are optional

Summary by CodeRabbit

  • Refactor
    • Node lookup and caching logic streamlined to always use a consistent identifier, improving reliability of reference discovery.
  • Bug Fixes
    • More consistent behavior when item identifiers are missing, reducing missed or duplicated reference suggestions and improving canvas/reference rendering.

✏️ Tip: You can customize this high-level summary in your review settings.

@linear
Copy link

linear bot commented Jan 10, 2026

@supabase
Copy link

supabase bot commented Jan 10, 2026

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

Copy link
Collaborator Author

sid597 commented Jan 10, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@sid597 sid597 changed the title findDiscourseNode accepts title as well ENG-1266: Bug: Component not loading on any page Jan 10, 2026
@sid597 sid597 marked this pull request as ready for review January 10, 2026 17:35
Copy link
Collaborator Author

sid597 commented Jan 10, 2026

@CodeRabbit ai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 10, 2026

@sid597: I'll conduct a full review of this PR. I can see this addresses a bug in findDiscourseNode where the function incorrectly exits when uid is not passed, even though both uid and title are optional parameters. I'll review all the changes to ensure the fix properly handles the optional parameters.


🧠 Learnings used
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:20.007Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.

Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 10, 2026

📝 Walkthrough

Walkthrough

findDiscourseNode's parameter uid was changed from optional to required (uid?: stringuid: string); the function no longer early-returns for undefined uid. Call sites were updated to pass a string (often uid || ""). One caller switched from title-only lookup to supplying a page UID.

Changes

Cohort / File(s) Summary
findDiscourseNode implementation
apps/roam/src/utils/findDiscourseNode.ts
Signature changed to require uid: string; removed early undefined-uid guard; caching/lookup now assumes a string uid (cache key/storage behavior updated accordingly).
Canvas event handler
apps/roam/src/components/canvas/Tldraw.tsx
Calls to findDiscourseNode now pass `uid: e.detail.uid
Formatting utilities
apps/roam/src/utils/formatUtils.ts
getReferencedNodeInFormat updated to call `findDiscourseNode({ uid: uid
Observers / listeners
apps/roam/src/utils/initializeObserversAndListeners.ts
Replaced title-only node lookup with uid-based lookup: findDiscourseNode({ uid, title }) where uid is obtained via getPageUidByPageTitle(title); removed prior nodes-list retrieval for this call.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: fixing a bug where findDiscourseNode incorrectly exits when uid is not provided, making the component fail to load.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 373c15f and 76f23d1.

📒 Files selected for processing (4)
  • apps/roam/src/components/canvas/Tldraw.tsx
  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefer type over interface in TypeScript
Use explicit return types for functions
Avoid any types when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability

Files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
apps/roam/**/*.{js,ts,tsx,jsx,json}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

Prefer existing dependencies from package.json when working on the Roam Research extension

Files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension

Files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
apps/roam/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality

Files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
apps/roam/**

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

Implement the Discourse Graph protocol in the Roam Research extension

Files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
🧠 Learnings (10)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:20.007Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In PostgreSQL queries within the discourse-graph project, adding `LIMIT 1` to `auth.uid()` lookups provides positive performance impact and should be retained as an optimization technique.
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
📚 Learning: 2025-12-07T20:54:20.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:20.007Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.

Applied to files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.

Applied to files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.

Applied to files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.

Applied to files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.

Applied to files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.

Applied to files:

  • apps/roam/src/utils/findDiscourseNode.ts
  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/utils/formatUtils.ts
📚 Learning: 2025-06-25T22:56:17.522Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.

Applied to files:

  • apps/roam/src/utils/initializeObserversAndListeners.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.

Applied to files:

  • apps/roam/src/utils/initializeObserversAndListeners.ts
  • apps/roam/src/components/canvas/Tldraw.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality

Applied to files:

  • apps/roam/src/components/canvas/Tldraw.tsx
🧬 Code graph analysis (1)
apps/roam/src/utils/findDiscourseNode.ts (1)
apps/roam/src/utils/getDiscourseNodes.ts (1)
  • DiscourseNode (14-35)
🔇 Additional comments (4)
apps/roam/src/utils/findDiscourseNode.ts (1)

11-11: Signature change looks correct, but the caching behavior with empty strings is worth noting.

The change from uid?: string to uid: string correctly addresses the bug where the function would exit early on undefined uid. However, note that when callers pass uid || "" (empty string), caching is skipped because "" is falsy in JavaScript (line 25). This is actually the intended behavior based on the learnings - uid is the cache key, so skipping cache for empty uid is appropriate.

Also applies to: 25-25

apps/roam/src/utils/formatUtils.ts (1)

139-139: LGTM!

The call site correctly adapts to the updated findDiscourseNode signature by using uid || "" to ensure a string is always passed. This preserves the existing behavior where an undefined uid results in no caching.

apps/roam/src/components/canvas/Tldraw.tsx (1)

481-484: LGTM!

The call correctly handles the optional e.detail.uid by defaulting to an empty string, maintaining compatibility with the updated findDiscourseNode signature while preserving the existing behavior.

apps/roam/src/utils/initializeObserversAndListeners.ts (1)

98-100: Improved node lookup with proper UID derivation.

The change correctly derives uid from the page title using getPageUidByPageTitle(title) and passes both uid and title to findDiscourseNode. This aligns with the caching pattern where uid is the cache key. Based on learnings, getPageUidByPageTitle is synchronous and returns an empty string if the page doesn't exist, which is handled correctly by the updated findDiscourseNode.

The uid variable is also reused on line 111 for renderDiscourseContextAndCanvasReferences, avoiding a redundant lookup.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sid597 sid597 requested a review from mdroidian January 10, 2026 18:31
…e related components to handle uid correctly. This improves type safety and prevents potential undefined errors.
@mdroidian
Copy link
Contributor

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 12, 2026

✅ Actions performed

Full review triggered.

@mdroidian
Copy link
Contributor

@sid597 I was working on another PR and also came across this. I went a different direction.

@mdroidian
Copy link
Contributor

#559 made uid optional and returned early if no uid.


const nodes = getDiscourseNodes();
const node = findDiscourseNode({ title, nodes });
const uid = getPageUidByPageTitle(title);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing UID lets us use the findDiscourseNode cache, at the tradeoff of running a pull on every page. Queries on every page are not ideal, but this pull is pretty insignificant.

@mdroidian mdroidian merged commit e4ff23e into main Jan 12, 2026
6 checks passed
@mdroidian mdroidian deleted the eng-1266-component-not-loading-on-any-page branch January 12, 2026 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants